-
Notifications
You must be signed in to change notification settings - Fork 92
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
R: Add snapshot tests for indentation #2577
Conversation
@@ -0,0 +1,5 @@ | |||
Launch tests by running this from the repository root: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like having a README
import * as fs from 'fs'; | ||
import { CURSOR, type, withFileEditor } from './editor-utils'; | ||
|
||
const snapshotsFolder = `${__dirname}/../../src/test/snapshots`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I recently eliminated these sorts of inline ad hoc paths in favor of using EXTENSION_ROOT_DIR
everywhere possible inside the extension:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, done!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is really exciting to get some of these behaviors under test. 🎉
A couple of minor questions/thoughts:
- Let's make the change Jenny suggested for the path.
- I looked through a few extensions and didn't see this kind of snapshot or "golden" test used elsewhere. For our information, did you find any? Not because I think we should change anything, but because I'd like to see what kinds of practices people use.
fd23d44
to
5032416
Compare
I did not find any, they seem to prefer writing snippets inline in the typescript code. I think the snapshot approach implemented here has less friction for adding cases though. |
This adds snapshot tests for indentation issues described in:
Closes #1694.
I initially tried to import the indentation test cases from ESS (https://github.com/emacs-ess/ESS/blob/master/test/styles/RStudio-.R). However those tests assume you can do something like the reindent lines action.
Reindent lines doesn't work here because it only uses regular indentation rules without on-enter rules, which isn't consistent with what happens when the user types Enter.
Pasting with auto-indent doesn't work either because it doesn't match the behaviour the user would see when typing the code manually. For instance, typing a closing delimiter like
)
triggers a "matching indent", seepositron/src/vs/editor/common/cursor/cursorTypeOperations.ts
Lines 947 to 963 in 06743ca
Typing the whole file in a loop one character at a time doesn't work either, because typing an opening delimiter like
{
auto-closes it, which corrupts the snapshots. Disabling auto-closing doesn't help because without the closing delimiters, we can't reproduce some of the regexp rules sensitive to those delimiters.After struggling a while I gave up on this approach. I think we need to test only one action per snapshot case. This is what is implemented in the
indentation-cases.R
(inputs) andindentation-snapshots.R
(outputs) files. These contain a list of fenced snapshots with a special marker indicating the cursor position. The snapshots are inserted in an editor and a newline is typed at the cursor. The result is pushed to the snapshot files.The infrastructure for this is based on the Typescript tests for indentation that @juliasilge discovered: https://github.com/posit-dev/positron/blob/main/extensions/typescript-language-features/src/test/unit/onEnter.test.ts#L36C1-L36C1. These are disabled because of intermittent failures on Windows so we'll have to watch out for that.
I've added a VS Code workspace inside the test folder to hold overridden settings. That's a bit of a heavier setup that I'd like but I don't think it's possible to create a virtual context where we could override settings for unit tests?
In terms of workflow these snapshots are in line with the way testthat handled snapshots pre edition 3. The snapshots are always regeneratedv If they don't match this is an error, but rerunning the test will no longer fail as the snapshots have been updated. The differences should be inspected with git and either accepted if the new output is correct, or discarded if it's not.